Audio: IGO_NR: Convert to module adapter #8301

singalsu · 2023-10-10T16:47:55Z

This is first "working" draft IPC4 version, but processing with stub library is forced to 48 frames for 1 ms LL scheduling. It obviously can't be used with the real processing library. For 16 ms need to change this further to support DP mode in IPC4.

src/audio/igo_nr/igo_nr.c

lgirdwood

Looks clean.

src/audio/igo_nr/igo_nr.c

lyakh

that's a lot of rework!

src/audio/igo_nr/igo_nr.c

src/include/sof/audio/igo_nr/igo_nr_comp.h

src/audio/igo_nr/igo_nr.c

src/include/sof/audio/igo_nr/igo_nr_comp.h

src/audio/igo_nr/igo_nr.c

btian1

that's definitely not one PR work, right? title is convert to module interface, inside PR, at least 3 tasks are done, do you want to split it to several PRs? for better review/merge/track?

src/audio/igo_nr/igo_nr.c

src/audio/igo_nr/igo_nr_stub.c

singalsu · 2023-12-12T12:00:45Z

that's definitely not one PR work, right? title is convert to module interface, inside PR, at least 3 tasks are done, do you want to split it to several PRs? for better review/merge/track?

The problem is that the component didn't work even with IPC3. It's impossible to test without all the changes included. Since this is a closed component and not useful without licensing it, I'd like to pass this with small effort to make this testable again. The owner or licensors of this should do the extra polishing effort.

lgirdwood · 2023-12-18T16:39:58Z

that's definitely not one PR work, right? title is convert to module interface, inside PR, at least 3 tasks are done, do you want to split it to several PRs? for better review/merge/track?

The problem is that the component didn't work even with IPC3. It's impossible to test without all the changes included. Since this is a closed component and not useful without licensing it, I'd like to pass this with small effort to make this testable again. The owner or licensors of this should do the extra polishing effort.

Ack - the aim here is not to change functionality or improve performance, but to do the mechanical work to bring it to the module API. Once at module API, it can then be integrated by folks who work on this module.

lyakh · 2023-12-19T13:57:00Z

src/audio/igo_nr/igo_nr.c

+	case SOF_IPC4_SWITCH_CONTROL_PARAM_ID:
+		comp_info(dev, "igo_nr_cmd_set_config(), SOF_IPC4_SWITCH_CONTROL_PARAM_ID");
+		return igo_nr_set_chan(mod, ctl);
+		return 0;


redundant line

Oops, wasn't fixed. Done now.

src/audio/igo_nr/igo_nr.c

btian1 · 2023-12-20T07:07:40Z

src/audio/igo_nr/igo_nr.c

+		ptr = (char *)ptr - cir_buf_size;
+
+	return ptr;
+}


static inline void *cir_buf_wrap(void *ptr, void *buf_addr, void *buf_end)
{
if (ptr >= buf_end)
ptr = (char *)buf_addr +
((char *)ptr - (char *)buf_end);

assert((intptr_t)ptr <= (intptr_t)buf_end); return ptr;

}
why not directly use this?

I don't see issues in what I proposed.

check https://github.com/thesofproject/sof/blob/main/src/include/sof/audio/audio_stream.h#L414
you don't need define a new one.

OK, I've changed and tested it now - pushing new version!

src/audio/igo_nr/igo_nr.c

tools/rimage/config/lnl.toml

lgirdwood · 2023-12-20T10:18:48Z

@lyakh @btian1 we only need to focus review on the changes to use the module API. This PR is not about improving the existing IGO code as we need to minimize the change surface here to module API integration only. So yes, there are lots of ways the IGO code can be improved, but we need to get integration with module API done before any other code improvements. Same goes for all 3P modules integrations to module API.

singalsu · 2024-01-04T13:31:38Z

I rebased and fixed rimage related conflicts. This version sounds choppy in playback test (sof-hda-benchmark-igo_nr32-tgl.tplg).

singalsu · 2024-01-04T15:32:31Z

I rebased and fixed rimage related conflicts. This version sounds choppy in playback test (sof-hda-benchmark-igo_nr32-tgl.tplg).

Correction, the issue was in my test setup: DUT -> USB sound card -> PC. Hooking headphones directly to DUT fixed the choppy playback. Also I was able to fix the choppy sounding test setup afterwards. So, this version is OK.

Also note for testing. Since there is no DP scheduler for TGL, I'm not able to process 768 frame size (16 ms), so I've made this code change (not in PR) just for my own DUT test run:

diff --git a/src/include/sof/audio/igo_nr/igo_nr_comp.h b/src/include/sof/audio/igo_nr/igo_nr_comp.h
index 82bf9f2d3..b1ba5f0d1 100644
--- a/src/include/sof/audio/igo_nr/igo_nr_comp.h
+++ b/src/include/sof/audio/igo_nr/igo_nr_comp.h
@@ -13,7 +13,7 @@
 #include <sof/audio/igo_nr/igo_lib.h>
 #include <user/igo_nr.h>
 
-#define IGO_FRAME_SIZE (768)
+#define IGO_FRAME_SIZE (48)
 #define IGO_NR_IN_BUF_LENGTH (IGO_FRAME_SIZE)
 #define IGO_NR_OUT_BUF_LENGTH (IGO_FRAME_SIZE)

Need to address the DP rimage mode for MTL and example topology later. In TGL changing the period in topology to 16 ms has no impact since IPC4 assumes 1 ms for LL scheduling.

btian1

LGTM, only one open left.

btian1 · 2024-01-16T13:29:17Z

src/audio/igo_nr/igo_nr.c

+		ptr = (char *)ptr - cir_buf_size;
+
+	return ptr;
+}


check https://github.com/thesofproject/sof/blob/main/src/include/sof/audio/audio_stream.h#L414
you don't need define a new one.

lgirdwood · 2024-01-17T13:20:22Z

@singalsu conflicts, probably since RTNR merged.

This patch converts this component to module adapter API. - New() becomes simpler init() - Params() is handled in module adapter, in init() "mod->verify_params_flags = BUFF_PARAMS_RATE;" replaces igo_nr_verify_params(). - Rates check in params() is placed to separate function igo_nr_check_params() to be called from prepare(). - cmd() handler is changed to module adapter client style with igo_nr_set_config() and igo_nr_get_config(). - igo_nr_process() and igo_nr_copy() are merged to module API style igo_nr_process() function. - igo_nr_prepare() sets source and sink align constraints for processing. - igo_nr_trigger() is removed, handled by module adapter. - Changes to support IPC4. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Function igo_nr_init() expects a nonzero value for rballoc() size so this can't be left zero. The value is just something for stub usage that felt suitable for an unknown. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This allows to build and load IGO NR stub version to testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The source sink API is needed for DP (data processing) scheduler that is needed for 16 ms schedule rate. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch enables to load the component in the changed platforms. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu requested a review from fuyuntsuo October 10, 2023 16:52

singalsu commented Oct 10, 2023

View reviewed changes

src/audio/igo_nr/igo_nr.c Show resolved Hide resolved

singalsu commented Oct 11, 2023

View reviewed changes

src/audio/igo_nr/igo_nr.c Show resolved Hide resolved

lgirdwood reviewed Oct 17, 2023

View reviewed changes

src/audio/igo_nr/igo_nr.c Show resolved Hide resolved

singalsu force-pushed the igonr_module_convert branch from 9918c61 to cd4e32a Compare October 27, 2023 16:36

lyakh reviewed Oct 30, 2023

View reviewed changes

src/audio/igo_nr/igo_nr.c Outdated Show resolved Hide resolved

src/audio/igo_nr/igo_nr.c Outdated Show resolved Hide resolved

src/audio/igo_nr/igo_nr.c Outdated Show resolved Hide resolved

singalsu force-pushed the igonr_module_convert branch from cd4e32a to 5d0fd84 Compare November 1, 2023 16:30

singalsu marked this pull request as ready for review November 2, 2023 12:59

singalsu requested review from ranj063, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners November 2, 2023 12:59

singalsu requested review from lyakh and lgirdwood November 2, 2023 13:01

lyakh reviewed Nov 3, 2023

View reviewed changes

btian1 reviewed Nov 6, 2023

View reviewed changes

src/audio/igo_nr/igo_nr.c Show resolved Hide resolved

singalsu force-pushed the igonr_module_convert branch from 5d0fd84 to 2af73d8 Compare November 6, 2023 10:45

singalsu requested review from lyakh and btian1 November 6, 2023 10:45

singalsu force-pushed the igonr_module_convert branch from 2af73d8 to 2a18630 Compare November 6, 2023 10:54

lyakh reviewed Nov 7, 2023

View reviewed changes

src/audio/igo_nr/igo_nr.c Show resolved Hide resolved

src/include/sof/audio/igo_nr/igo_nr_comp.h Show resolved Hide resolved

src/audio/igo_nr/igo_nr.c Outdated Show resolved Hide resolved

btian1 reviewed Nov 10, 2023

View reviewed changes

src/audio/igo_nr/igo_nr.c Show resolved Hide resolved

src/audio/igo_nr/igo_nr.c Show resolved Hide resolved

src/audio/igo_nr/igo_nr_stub.c Show resolved Hide resolved

kv2019i added this to the v2.9 milestone Nov 29, 2023

kv2019i mentioned this pull request Nov 29, 2023

[FEATURE] Update and remove passthrough definitions #4487

Open

singalsu force-pushed the igonr_module_convert branch from 2a18630 to ffca82f Compare December 19, 2023 12:22

singalsu requested a review from lyakh December 19, 2023 12:22

singalsu requested a review from btian1 December 19, 2023 12:22

lyakh reviewed Dec 19, 2023

View reviewed changes

btian1 reviewed Dec 20, 2023

View reviewed changes

singalsu force-pushed the igonr_module_convert branch from ffca82f to 4b17596 Compare January 4, 2024 13:27

singalsu force-pushed the igonr_module_convert branch from 4b17596 to 582836c Compare January 12, 2024 15:09

singalsu requested review from lyakh and btian1 January 12, 2024 15:10

singalsu force-pushed the igonr_module_convert branch from 582836c to 4150713 Compare January 12, 2024 15:32

btian1 reviewed Jan 16, 2024

View reviewed changes

singalsu force-pushed the igonr_module_convert branch from 4150713 to 419e505 Compare January 16, 2024 15:27

singalsu requested a review from btian1 January 16, 2024 15:30

btian1 approved these changes Jan 17, 2024

View reviewed changes

singalsu force-pushed the igonr_module_convert branch from 419e505 to 068240b Compare January 17, 2024 11:13

singalsu added 5 commits January 17, 2024 17:49

Tools: Testbench: Build IGO_NR with stub for testbench

37c18f5

This allows to build and load IGO NR stub version to testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Audio: IGO_NR: Convert to source sink API

f5e3629

The source sink API is needed for DP (data processing) scheduler that is needed for 16 ms schedule rate. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Tools: Rimage: Config: Add IGO NR component for TGL, MTL, LNL platforms

3685642

This patch enables to load the component in the changed platforms. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu force-pushed the igonr_module_convert branch from 068240b to 3685642 Compare January 17, 2024 16:03

lgirdwood approved these changes Jan 18, 2024

View reviewed changes

lgirdwood merged commit 7b5475b into thesofproject:main Jan 18, 2024
44 of 46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio: IGO_NR: Convert to module adapter #8301

Audio: IGO_NR: Convert to module adapter #8301

singalsu commented Oct 10, 2023 •

edited

Loading

lgirdwood left a comment

lyakh left a comment

btian1 left a comment

singalsu commented Dec 12, 2023 •

edited

Loading

lgirdwood commented Dec 18, 2023

lyakh Dec 19, 2023

singalsu Jan 12, 2024

singalsu Jan 17, 2024

btian1 Dec 20, 2023

singalsu Jan 12, 2024

btian1 Jan 16, 2024

singalsu Jan 16, 2024

lgirdwood commented Dec 20, 2023

singalsu commented Jan 4, 2024

singalsu commented Jan 4, 2024

btian1 left a comment

btian1 Jan 16, 2024

lgirdwood commented Jan 17, 2024

Audio: IGO_NR: Convert to module adapter #8301

Audio: IGO_NR: Convert to module adapter #8301

Conversation

singalsu commented Oct 10, 2023 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

btian1 left a comment

Choose a reason for hiding this comment

singalsu commented Dec 12, 2023 • edited Loading

lgirdwood commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Dec 20, 2023

singalsu commented Jan 4, 2024

singalsu commented Jan 4, 2024

btian1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Jan 17, 2024

singalsu commented Oct 10, 2023 •

edited

Loading

singalsu commented Dec 12, 2023 •

edited

Loading